Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drawio - Add resolution of external image to get self contained SVG #1771

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jynolen
Copy link

@jynolen jynolen commented Aug 13, 2024

This PR help to keep drawio svg self contained.

Background
I use SelfHosted Kroki with antora doc generator
When using drawio diagrams it happned that some part of the diagr was missing because it relies on external images.

What have been done:

With this PR, during the rendering phase, kroki will retreive external image to embed them as base64 src.
This way diagrram will be rendered correctly whenever and wherever

@jynolen jynolen changed the title feat: add image resolution Drawio - Add resolution of external image to get self contained SVG Aug 13, 2024
@jynolen jynolen marked this pull request as ready for review August 13, 2024 13:19
Comment on lines 53 to 54
await writeFile("before.svg", svgRoot)
await writeFile("after.svg", $("svg").toString())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this was for testing purpose?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I will remove it

@@ -27,6 +29,32 @@ export class Worker {
this.convertTimeout = process.env.KROKI_DIAGRAMSNET_CONVERT_TIMEOUT || '15000'
}

async resolveImage(svgRoot) {
let $ = cheerio.load(svgRoot, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are using Puppeteer we could potentially parse/edit the SVG in page.evaluate? That would remove the dependency on cheerio.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely I am not a JS expert I will check to change it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you find a way to implement this feature without relying on cheerio? I can take a look if you want?

@ggrossetie
Copy link
Member

Someone mentioned in the community chat that diagrams.net has an "embed" feature:

https://www.drawio.com/doc/faq/embed-mode

It might be possible to rely on the query parameter embed=1 to enable it (or find a way to set this flag).

@ggrossetie
Copy link
Member

@jynolen could you please share an example with external images? I want to test if embed=1 is working as intended.

@jynolen
Copy link
Author

jynolen commented Oct 22, 2024

Here you are 👍 https://gist.github.com/jynolen/05f48d96f09cfdffd9bcf6b99b91256f
I put the two (with url and with b64)

@ggrossetie
Copy link
Member

It's not as easy as I thought since it relies on a proxy: #1796
So your strategy is probably the best.

As this adds an attack vector, I think it would be preferable to enable this feature when Kroki safe mode is set to unsafe: https://docs.kroki.io/kroki/setup/configuration/#_safe_mode

@jynolen
Copy link
Author

jynolen commented Dec 2, 2024

I update the code to discard unsage of cheerio.

I put 2 environment variables "KROKI_SAFE_MODE" to allow / disallow fetching images, and "KROKI_DIAGRAMNET_PORT" to customise listening port

I take the liberty to rewrite a bit the evaluation function to make it a bit easier to read and maintain.

@ggrossetie If you can have a look

@jynolen jynolen force-pushed the feat/resolveImage branch 3 times, most recently from 88014da to 0acbc99 Compare December 3, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants